Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(brew-bump.sh): add checks and handle errors #4236

Merged
merged 2 commits into from
Sep 28, 2021
Merged

fix(brew-bump.sh): add checks and handle errors #4236

merged 2 commits into from
Sep 28, 2021

Conversation

jsjoeio
Copy link
Contributor

@jsjoeio jsjoeio commented Sep 23, 2021 •
edited
Loading

This PR adds a lot of bash helper functions (+ tests) used in brew-bump.sh in order to ensure this script doesn't fail when publishing a release.

image

TODOs

  • add BATS test for steps-lib.sh
  • add check + test for $VERSION
  • add check for cdr/ci/homebrew
  • add check for remote upstream
  • add check for $HOMEBREW_GITHUB_API_TOKEN
  • add check for git-ask-pass.sh
  • add check for cleaning up homebrew-core
  • fix Trivy vulnerability (chore(deps): upgrade ansi-regex to 5.0.1 and tmpl to 1.0.5 #4251)
  • test the GIT_ASKPASS in isolated environment

Fixes #3962

Test Plan

To test out the other parts of the script (specifically GIT_ASKPASS), here is what I did:

  1. Create new Workspace on https://master.cdr.dev/workspaces
  2. "log out" of git with:
git config --global --unset user.name
git config --global --unset user.email
git config --global --unset credential.helper
  1. Verify log out by trying to clone via HTTPS a private repo:
git clone https://github.com/jsjoeio/dip.chat-v2.git
  1. Create script similar to brew-bump.sh and add steps-lib.sh to Workspace.
  2. Make script executable chmod +x test-brew.sh
  3. Run script: VERSION="1.2.3" HOMEBREW_GITHUB_API_TOKEN="<redcated>" ./test-brew.sh (make sure the token had repo and workflow in scope).

Notes

Ran into issues with scripts that used set -u so that's why I created a new file called steps-lib.sh.

See: sstephenson/bats#171

@jsjoeio jsjoeio self-assigned this Sep 23, 2021
@jsjoeio jsjoeio added chore Related to maintenance or clean up ci Issues related to ci testing Anything related to testing labels Sep 23, 2021
@codecov
Copy link

codecov bot commented Sep 23, 2021 •
edited
Loading

Codecov Report

Merging #4236 (9aac550) into main (8f72481) will not change coverage.
The diff coverage is n/a.

❗ Current head 9aac550 differs from pull request most recent head 8ef950a. Consider uploading reports for the commit 8ef950a to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main    #4236   +/-   ##
=======================================
  Coverage   65.09%   65.09%           
=======================================
  Files          36       36           
  Lines        1882     1882           
  Branches      380      380           
=======================================
  Hits         1225     1225           
  Misses        559      559           
  Partials       98       98

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f72481...8ef950a. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Sep 23, 2021 •
edited
Loading

✨ Coder.com for PR #4236 deployed! It will be updated on every commit.

@jsjoeio jsjoeio force-pushed the jsjoeio/fix-brew-bump branch 2 times, most recently from b590dcd to 9aac550 Compare September 27, 2021 23:23
@jsjoeio jsjoeio marked this pull request as ready for review September 27, 2021 23:24
@jsjoeio jsjoeio requested a review from a team as a code owner September 27, 2021 23:24
@jsjoeio jsjoeio changed the title fix: add checks and handle errors brew-bump.sh fix(brew-bump.sh): add checks and handle errors Sep 27, 2021
jawnsy
jawnsy approved these changes Sep 27, 2021
View reviewed changes
Copy link
Contributor

@jawnsy jawnsy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left some really minor suggestions for your consideration, this is good-to-go as it is

ci/steps/brew-bump.sh Outdated Show resolved Hide resolved
ci/steps/brew-bump.sh Outdated Show resolved Hide resolved
ci/steps/brew-bump.sh Show resolved Hide resolved
ci/steps/brew-bump.sh Show resolved Hide resolved
test/scripts/steps-lib.bats Outdated

@test "is_env_var_set should return 1 if env var is not set" {
run is_env_var_set "ASDF_TEST_SET"
[ "$output" = 1 ]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you change the functions to return stuff instead of echo, then you'd have to change this to check the $status instead of $output

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! Okay if/after I refactor, I'll update these!

@jsjoeio
fix: add checks and handle errors brew-bump.sh
e5f03e0 
feat(script): add steps-lib, is_env_var_set & test

feat(brew-bump): add check for VERSION

feat(brew-bump): check HOMEBREW_GITHUB_API_TOKEN

feat(steps-lib): add directory_exists helper fn

fix(brew-bump): check that git clone worked

feat(brew-bump): add check for remote upstream

fix: remove upstream command thing

feat(steps-lib): add file_exists helper function

feat(brew-bump): add check for git-askpass.sh

feat(steps-lib): add is_executable function & test

feat(brew-bump): add check for is_executable

refactor: use GIT_ASKPASS as variable
@jsjoeio
feat(brew-bump): add check for cleanup step
8ef950a
@jsjoeio jsjoeio force-pushed the jsjoeio/fix-brew-bump branch from 9aac550 to 8ef950a Compare September 28, 2021 23:07
@jsjoeio jsjoeio enabled auto-merge September 28, 2021 23:11
@jsjoeio jsjoeio merged commit 999960e into main Sep 28, 2021
@jsjoeio jsjoeio deleted the jsjoeio/fix-brew-bump branch September 28, 2021 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jawnsy jawnsy jawnsy approved these changes

Assignees

@jsjoeio jsjoeio

Labels
chore Related to maintenance or clean up ci Issues related to ci testing Anything related to testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix brew-bump.sh script
2 participants
@jsjoeio @jawnsy